Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[8.x] Adds new RequestSent and ResponseReceived events to the HTTP Client #37572

Merged
merged 9 commits into from
Jun 3, 2021
Merged

[8.x] Adds new RequestSent and ResponseReceived events to the HTTP Client #37572

merged 9 commits into from
Jun 3, 2021

Conversation

lukeraymonddowning
Copy link
Contributor

@lukeraymonddowning lukeraymonddowning commented Jun 2, 2021 •
edited
Loading

Hey guys,

This PR adds two new events, RequestSent and ResponseReceived, which are dispatched by the Http Client.

RequestSent

The RequestSent event is fired when any request is made when calling any of the following Http Client methods: 'get', 'head', 'post', 'put' and 'delete'. It receives the HTTP method, URL, and any sent options.

ResponseReceived

The ResponseReceived event is fired when a successful response is returned from the Http Client. It receives the Response instance.

What are the benefits?

These two events allow a 3rd party package to listen for and react to HTTP Client requests. I can see it being used for debugging, logging, performance monitoring, service downtime analysis and more.

I decided to contribute this when I went to create a PR for the spatie/laravel-ray package to allow monitoring of Http Client requests only to realise there wasn't a way to do so.

As always, thank you very much for all the hard work and for considering my PR.

Regards,
Luke

@lukeraymonddowning lukeraymonddowning changed the title Adds new RequestSent and ResponseReceived events to the HTTP Client. [8.x] Adds new RequestSent and ResponseReceived events to the HTTP Client. Jun 2, 2021
@taylorotwell
Copy link
Member

Renamed to RequestSending (it is dispatched slightly before the request is actually sent), and moved its location to sendRequest method in case of promise usage. Otherwise, the event would have fired before the request was actually sending, which could have been confusing.

Thoughts?

@GrahamCampbell
Copy link
Member

Why not include the request object in the first event?

@taylorotwell
Copy link
Member

taylorotwell commented Jun 2, 2021 •
edited
Loading

I agree having the full Illuminate\Http\Client\Request object would be useful in both events to be honest.

@lukeraymonddowning
Copy link
Contributor Author

@taylorotwell agreed. Also agreed on the received event having the response object. I'll refactor when I'm back at my keyboard.

@GrahamCampbell GrahamCampbell changed the title [8.x] Adds new RequestSent and ResponseReceived events to the HTTP Client. [8.x] Adds new RequestSent and ResponseReceived events to the HTTP Client Jun 2, 2021
@lukeraymonddowning
Copy link
Contributor Author

Okay @taylorotwell @GrahamCampbell, I've moved the firing of the RequestSending event to a beforeSendingCallback so that we can include the Request object.

I've also made the ResponseReceived event accept the Request too, so that you have both the request and response in that event. I've done this by setting the request in a property on the PendingRequest, which seemed the cleanest way to achieve this. Can you see any issues with that approach, or does that seem good to you?

@lukeraymonddowning
Copy link
Contributor Author

Also @taylorotwell, completely understand if not (because seeing 'Request' as the first word makes sense in one way), but would it be worth considering SendingRequest as the event name, just so that it reads better as a phrase? Not precious, but thought it was worth raising as a question.

@taylorotwell taylorotwell merged commit 065e130 into laravel:8.x Jun 3, 2021
@lukeraymonddowning lukeraymonddowning deleted the http-client-events branch June 3, 2021 13:32
@gdebrauwer gdebrauwer mentioned this pull request Jun 6, 2021
Merged
@louisgab louisgab mentioned this pull request Jul 5, 2021
Merged
@viniciushss-interstar viniciushss-interstar mentioned this pull request Jul 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driesvints driesvints driesvints left review comments

@GrahamCampbell GrahamCampbell Awaiting requested review from GrahamCampbell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukeraymonddowning @taylorotwell @GrahamCampbell @driesvints